Fixes #705: Handle renamed tag object type column in migration 0006 - #725

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

Claude finished @arthanson's task in 3m 55s —— View job

Review

• Read changed files
• Analyze migration logic
• Analyze test coverage
• Post review findings

Migration (0006_tag_object_types.py)

Index lookup inconsistency with 0003_rename_indexes

0003 matches by LIKE: AND indexdef LIKE %s with f'% {definition}'. 0006 uses split_part / starts_with. Both are semantically equivalent for well-formed pg_indexes entries, but diverging from the established pattern makes the two harder to reason about together. Not a bug, but worth aligning for future maintainers.

Unquoted identifiers in DDL statements (0006_tag_object_types.py:58, :72)

cursor.execute(f"ALTER INDEX {schema_name}.{index.indexname} RENAME TO {result[0]}")
cursor.execute(f"ALTER TABLE {schema_table} RENAME COLUMN contenttype_id TO objecttype_id")

schema_name is derived from plugin config + a UUID-based schema ID, and index.indexname / result[0] come from pg_indexes. Practically low-risk, but neither schema_name nor the index names are wrapped in psycopg2.sql.Identifier / psycopg2.sql.SQL. The existing 0003 migration has the same pattern, so this isn't new, but it's worth noting if there's ever a path for schema_prefix to contain special characters.

schema_name queried with an f-string on line 26–28

The table-existence check at the top of the loop uses an f-string rather than parameterized SQL, while the column-existence check below correctly uses %s. Minor inconsistency that pre-dates this PR but could be tidied up.

Tests (test_upgrade.py)

test_branch_predating_0131_can_apply_0131 has no skip guard for environments without extras.0131 (test_upgrade.py:338–356)

migration = loader.get_migration(*self.MIGRATION)

If this test ever runs against a NetBox version that doesn't include extras.0131 (whether during local development or a future NetBox version), get_migration raises KeyError with an opaque error. Consider:

loader = MigrationLoader(connection)
if self.MIGRATION not in loader.migrated_apps.get(self.MIGRATION[0], {}):
    self.skipTest(f"{self.MIGRATION} not present in this NetBox version")

Fix this →

test_branch_predating_0131_can_apply_0131 hardcodes operation selection (test_upgrade.py:346)

operation = next(op for op in migration.operations if op.model_name == 'tag' and op.name == 'object_types')

If NetBox ever restructures extras.0131, this raises StopIteration. Wrapping it in a try/except StopIteration with a self.skipTest(...) or self.fail(...) would make failures more debuggable.

_create_branch leaves orphaned connection aliases open

_create_branch creates a Branch ORM record (rolled back by TestCase's transaction) and a PostgreSQL schema (rolled back by transactional DDL). No branch connection is ever opened, so this is fine — but unlike the FastTeardownTransactionTestCase-based tests, there's no tearDown closing branch connections. If any path in copy_table or the subsequent operation.database_forwards call opens a branch connection, it would leak. Worth a quick check or a defensive tearDown.

test_copy_table_for_branch_predating_0131 branch count check (test_upgrade.py:333–336)

cursor.execute(f'SELECT COUNT(*) FROM "{schema}".{self.TABLE}')
branch_count = cursor.fetchone()[0]
cursor.execute(f'SELECT COUNT(*) FROM public.{self.TABLE}')
self.assertEqual(branch_count, cursor.fetchone()[0])

This is correct but only verifies row count parity. If the migration were to silently truncate the copied table but still write the same number of rows, this would pass. A stronger assertion (e.g., checksum or spot-checking a specific row) isn't strictly necessary here, but it's worth noting the check's limits.

Per Claude review:

1. Index lookup differs from 0003 (LIKE): Not fixed. LIKE treats _ as a wildcard and doesn't tell unique indexes from non-unique ones, so the exact match is safer.
2. Unquoted identifiers in DDL: Not fixed. This pattern predates the PR and is used across the plugin; the schema name comes from plugin config and the index names from pg_indexes.
3. f-string in the table-existence check: Fixed. The query now passes the schema and table names as parameters.
4. No skip guard for extras.0131: Not fixed. That migration shipped in NetBox 4.4.0 and the plugin requires 4.7 or later, and a skip would hide a real break.
5. StopIteration from the hardcoded operation lookup: Not fixed. The test error already points at that exact line.
6. _create_branch leaks connections: Not valid. The tests only use the default connection, and the search_path change is undone in a finally.
7. Row-count check is weak: Fixed. The test now seeds a tag (the table was empty before) and compares the branch's rows exactly with main's in both copy tests.